CI-260 - Transfer dataset files concurrently - #224
Merged
Conversation
Transfer time scaled with the number of files rather than total bytes, because each file was transferred one at a time and paid a large fixed cost before any bytes moved. - Run several file transfers at once, reporting into one aggregate progress bar rather than one bar per file. - Share a single boto3 transfer manager per client instead of building and tearing one down (~16 threads) for every file, and shut it down when the batch ends. - Upload by filename rather than by open file object, so s3transfer reads multipart chunks in parallel instead of serially. - Batch DataPortalFiles.download into one call; it previously rebuilt a boto3 session per file. Pass File objects through so their known sizes reach the transfer, saving a request per file. - Add --threads (default 8) to upload, download and resume-upload, and a threads parameter on the service and SDK methods. threads=1 is fully sequential with no threads anywhere, including inside boto3. An upload that exhausts its retries now raises and names every failed file, where it previously returned normally and left a partial dataset looking complete. Retry backoff is capped rather than growing to tens of minutes and blocking the remaining files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d-performance-61b806
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reduces its cognitive complexity from 23 to 4 by separating the three concerns it had combined: retrying a single transfer, running the set either sequentially or concurrently, and reporting the failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nathanthorpe
approved these changes
Aug 21, 2026
| self._client.download_file(bucket, key, absolute_path, | ||
| Callback=ProgressPercentage(progress), | ||
| ExtraArgs=self._download_args) | ||
| def upload_file(self, file_path: Path, bucket: str, key: str, |
Member
There was a problem hiding this comment.
We should preserve the "PathLike" behavior if possible, maybe we can detect if its a local file
Uploading by filename lets s3transfer read a file's parts in parallel, but it only works for real filesystem paths. #115 had switched to open()/ upload_fileobj precisely so Path-like objects backed by something else, such as an s3fs path, could be uploaded too. Check whether the path names a local file and pick accordingly: local files go through the shared transfer manager, anything else is streamed through its own open() as before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The file object path was going through client.upload_fileobj, which builds a transfer manager and its thread pools per file - the churn the shared manager was meant to remove, and worse once several files upload at once. s3transfer's manager accepts a filename or a seekable file object, so both paths can use one. Build it with create_transfer_manager and wrap it in S3Transfer for the filename case, keeping boto3's error translation, and submit file objects to the manager directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nathanthorpe
approved these changes
Aug 24, 2026
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Purpose
Speed up dataset upload and download. Transfer time previously scaled with the number of files rather than total bytes, because every file paid a large fixed cost and files were transferred strictly one at a time.
How
upload_directoryanddownload_directorynow resolve the work list, then run several transfers at once through aThreadPoolExecutor, reporting into one aggregate progress bar instead of one bar per file.upload_fileobj/download_fileeach built and tore down a boto3TransferManager(~16 threads) per file.S3Clientnow holds one for its lifetime and shuts it down when the batch ends. Local files are uploaded by filename so s3transfer reads their parts in parallel; Path-like objects that are not local files (see Change S3 Client upload implementation to use upload_fileobj #115) are streamed through their ownopen()into the same shared manager.DataPortalFiles.downloadcalleddownload_filesonce per file, constructing a freshboto3.Sessioneach time (~300 ms measured). It now makes a single batched call.Fileobjects are also passed through instead of being flattened to strings, which drops oneHeadObjectper file.--threadsto control it. Available onupload,downloadandresume-upload, and as athreadsparameter on the service and SDK methods. Default 8.--threads 1restores fully sequential behaviour: noThreadPoolExecutor, andTransferConfig(use_threads=False)so boto3 runs the transfer inline too — which also makes this usable where threads are unavailable, such as Pyodide.Behaviour changes
uniform(0,60) + retry*60, up to ~45 min and blocking every remaining file).Biggest risk
Concurrency is the new default, so upgrading changes behaviour for everyone. 8 files in flight could surface throttling or contention on networks and tenants unlike the one tested here, and failures that were previously serial and deterministic are now interleaved. Mitigation is
--threads N, with--threads 1restoring the old sequential behaviour.The risk I expected and ruled out: switching
upload_fileobj→upload_filecould have changed checksum semantics. It does not (verified below).Test dataset
301 files, 11.17 MB, uploaded to
dev.cirro.bio/ Pipeline Development as data typeFiles(custom_dataset):Both arms ran on the same venv and interpreter (Python 3.14.6, boto3 1.41.5, s3transfer 0.15.0), with
PYTHONPATHselecting whichcirroloaded, so the only variable is this diff. Released 1.12.1'sfile_utils.py,clients/s3.py,sdk/file.pyandservices/file.pyare byte-identical to this branch's merge-base, so the baseline is exactly "this branch minus these changes". 2 reps each, interleaved.Results
datasets.download_files(CLI path)dataset.download_files(SDK path)The SDK download path gains most because it also carried the per-file client rebuild: subtracting actual transfer time leaves ~90s of pure overhead on the baseline, or ~296 ms per file.
Correctness
ChecksumType=FULL_OBJECTand identicalCRC64NVMEvalues, for both the 4 KB file and the 10 MB multipart-threshold file.validate_folder: 301 matching, 0 not-matching, 0 missing, 0 errors.Connection pool is fullwarnings across all 12 runs.--threadsverified end to end.threads=1created zero worker threads and round-tripped 12/12 files;threads=8created 8 and round-tripped 12/12. Neither left threads behind.All benchmark datasets were deleted and the project reports none remaining.
Note on
--threads 1It removes every thread the transfer logic creates, but tqdm spawns its own monitor thread on the first progress bar. That is pre-existing (the released version does it too) and not affected by this flag.
🤖 Generated with Claude Code